fix: #1210 - Add error handling for missing dependencies #1232

mohammedsahl · 2020-06-19T19:38:49Z

Summary
This PR introduces an error handling method for missing dependencies as described in issue #1210 . onwarn intercepts warning messages and when message.code === 'UNRESOLVED_IMPORT' it means there is an unresolved or missing dependencies. Prior to this PR, a warning message would be logged to the console. The changes made throw an error instead, which can be handled by the appropriate error handler

Before:

lib/plugins/front-matter.min.js
lib/plugins/front-matter.js
lib/plugins/search.min.js
lib/plugins/search.js
'medium-zoom' is imported by src\plugins\zoom-image.js, but could not be resolved – treating it as an external dependency
'medium-zoom' is imported by src\plugins\zoom-image.js, but could not be resolved – treating it as an external dependency
lib/plugins/zoom-image.min.js
lib/plugins/zoom-image.js
No name was provided for external module 'medium-zoom' in output.globals – guessing 'mediumZoom'
No name was provided for external module 'medium-zoom' in output.globals – guessing 'mediumZoom'
lib/docsify.js
lib/docsify.min.js

After:

lib/plugins/front-matter.js
lib/plugins/front-matter.min.js
Error: Could not resolve module medium-zoom
    at Object.onwarn (C:\Users\simpl\Desktop\docsify\build\build.js:35:17)
    at Graph.onwarn (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\shared\index.js:87:25)
    at Graph.warn (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:17668:14)
    at ModuleLoader.handleMissingImports (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:17239:24)
    at ModuleLoader.<anonymous> (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:17288:26)
    at Generator.next (<anonymous>)
    at fulfilled (C:\Users\simpl\Desktop\docsify\node_modules\rollup\dist\rollup.js:40:28)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] build:js: `cross-env NODE_ENV=production node build/build.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] build:js script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\simpl\AppData\Roaming\npm-cache\_logs\2020-06-19T19_26_30_901Z-debug.log

Testing methdology

Run npm install; npm run build:js. The build should successful. This step is to ensure that there are no problems before deleteing a dependency
Take note of the dependencies in package.json
https://github.com/docsifyjs/docsify/blob/13f91a4fcb52fb7757e7ecbb4262fd89247dabdd/package.json#L55-70
We will select medium-zoom to be the test dependecy
Delete the appropriate directory from node_modules (node_modules/medium-zoom for our testing purposes)
Execute npm run build:js. An error message must be thrown, similar to the one above, labeled After
Run npm install; npm run build:js. The build should successful

What kind of change does this PR introduce? (check at least one)

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature
Related documents have been updated
Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

DO NOT include files inside lib directory.

vercel · 2020-06-19T19:38:54Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/gd5b2661n
✅ Preview: https://docsify-previe-git-fork-mlh-fellowship-1210-missing-depe-065e64.docsify-core.vercel.app

codesandbox-ci · 2020-06-19T19:48:25Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3a22fa4:

Sandbox	Source
nervous-curran-twvx3	Configuration

anikethsaha

LGTM

Thanks

trusktr

👍

build/build.js

anikethsaha · 2020-06-22T17:13:01Z

@trusktr is it fixing/closing the issue that is mentioned ?

jhildenbiddle

What other information is passed with the message object to onWarn()?

Ideally, we would check the path to verify that the unresolved import is something we want to import locally (i.e. from the src directory). Otherwise, this change will throw an error if we ever release an ES6 module that imports external resources. In other words, this change will address the issue today, but it could cause builds to break down the road.

anikethsaha · 2020-06-22T18:12:40Z

currently we need to check for modules that are installed.
if es6 comes, I guess then we want to check for both dep and local modules as well.

mohammedsahl · 2020-06-22T18:38:39Z

@jhildenbiddle A WarningHandler is also passed into the function as shown here

Warnings objects have, at a minimum, a code and a message property, allowing you to control how different kinds of warnings are handled. Other properties are added depending on the type of warning.

jhildenbiddle · 2020-06-22T23:30:30Z

@mohammedsahl --

Let's verify if onwarn() is called when a dependency is explicitly marked as external using rollup's external configuration option. This will help us determine how our custom onwarn() callback should behave.

Using the Before example from above as an example, try adding 'medium-zoom' (the name of the module that rollup cannot resolve) to the external array:

external: [
  'medium-zoom'
]

If onwarn() is not called, then rollup is smart enough to check the external array and realize that the unresolved module has been explicitly marked as an external dependency. This means we don't have to worry about issues with external dependencies in the future since adding them to the rollup's external array (which should be done anyway) will prevent onwarn() from being called. This also means that the onwarn() callback should always throw an error if message.code === UNRESOLVED_IMPORT (as it does now) because we know 1) either a module was not added to the external configuration or 2) a non-external dependency can not be resolved. The only change I would make here is to update the error message to handle both scenarios: Could not resolve module [message.source]. Try running 'npm install' or using rollup's 'external' option if this is an external dependency..

If onwarn() is called, then rollup is making no distinction between external dependencies explicitly listed in the external configuration and modules it simply cannot resolve. This means our onwarn() callback needs handle both scenarios by checking the message.source value against the external value(s) (or function -- see rollup docs) to determine if an error has actually occurred. The goal is to make sure we aren't throwing an error for a properly configured external dependency found in rollup's external configuration.

Finally, if the message object returned to onwarn() contains location information (per the rollup docs), let's include the path to the offending file in the console output (similar to rollup's default message). This will make life easier for devs if troubleshooting is necessary.

…:MLH-Fellowship/docsify into 1210-missing-dependencies-error-handling

mohammedsahl · 2020-06-23T16:07:05Z

@jhildenbiddle
Turns out that marking a dependency as external does not trigger onwarn(). I've made changes to the error message as you've suggested. Also added information about where the dependency is being imported

jhildenbiddle

Excellent!

Good to see rollup is doing the right thing. Much easier than handling the comparison with external values ourselves. I also think we’ve saved a future dev some headaches if/when core leveraged external dependencies.

Nice work, @mohammedsahl! :)

* develop: docs: removed codefund docs and plugin (#1262) docs: remove bundle size from the home page and documentation (#1257) fix: search can not search the table header (#1256) fix: after setting the background image, the button is obscured (#1234) Fix: fixed onlycover flag in mobile (#1243) fix: Updated docs with instructions for installing specific version (fixes #780) (#1225) fix: Add error handling for missing dependencies (fixes #1210) (#1232) [documdocs: deploy docsify in docker. (#1241) docs: Add embed gist instructions to Embed Files (fixes #932 ) (#1238) chore: add changelog 4.11.4 [build] 4.11.4 feat: added html sanitizer for remote rendering (#1128)

Add error handling for missing dependencies

df2b429

vercel bot deployed to Preview June 19, 2020 19:38 View deployment

anikethsaha requested review from anikethsaha, jhildenbiddle, Koooooo-7, sy-records and trusktr June 22, 2020 17:09

anikethsaha approved these changes Jun 22, 2020

View reviewed changes

trusktr previously approved these changes Jun 22, 2020

View reviewed changes

anikethsaha requested changes Jun 22, 2020

View reviewed changes

build/build.js Outdated Show resolved Hide resolved

Informative error message

c325212

mohammedsahl dismissed trusktr’s stale review via c325212 June 22, 2020 17:28

vercel bot deployed to Preview June 22, 2020 17:29 View deployment

anikethsaha previously approved these changes Jun 22, 2020

View reviewed changes

anikethsaha requested a review from trusktr June 22, 2020 17:37

jhildenbiddle linked an issue Jun 22, 2020 that may be closed by this pull request

[build] convert external dependencies (missing dependencies) into Rollup errors instead of warnings. #1210

Closed

jhildenbiddle requested changes Jun 22, 2020

View reviewed changes

Merge branch 'develop' into 1210-missing-dependencies-error-handling

b76dcdc

vercel bot deployed to Preview June 23, 2020 14:02 View deployment

mohammedsahl added 4 commits June 23, 2020 10:18

Add error handling for missing dependencies

f560d92

Informative error message

8009ecc

Merge branch '1210-missing-dependencies-error-handling' of github.com…

e8c68c0

…:MLH-Fellowship/docsify into 1210-missing-dependencies-error-handling

Add dependecy import location, error message changes

3a22fa4

mohammedsahl dismissed anikethsaha’s stale review via 3a22fa4 June 23, 2020 16:04

vercel bot deployed to Preview June 23, 2020 16:04 View deployment

jhildenbiddle approved these changes Jun 23, 2020

View reviewed changes

anikethsaha approved these changes Jun 23, 2020

View reviewed changes

anikethsaha merged commit 3673001 into docsifyjs:develop Jun 23, 2020

sy-records mentioned this pull request Aug 18, 2020

[Release]: v4.11.5 #1340

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #1210 - Add error handling for missing dependencies #1232

fix: #1210 - Add error handling for missing dependencies #1232

mohammedsahl commented Jun 19, 2020 •

edited

Loading

vercel bot commented Jun 19, 2020 •

edited

Loading

codesandbox-ci bot commented Jun 19, 2020 •

edited

Loading

anikethsaha left a comment

trusktr left a comment

anikethsaha commented Jun 22, 2020

jhildenbiddle left a comment

anikethsaha commented Jun 22, 2020

mohammedsahl commented Jun 22, 2020 •

edited

Loading

jhildenbiddle commented Jun 22, 2020 •

edited

Loading

mohammedsahl commented Jun 23, 2020

jhildenbiddle left a comment

fix: #1210 - Add error handling for missing dependencies #1232

fix: #1210 - Add error handling for missing dependencies #1232

Conversation

mohammedsahl commented Jun 19, 2020 • edited Loading

vercel bot commented Jun 19, 2020 • edited Loading

codesandbox-ci bot commented Jun 19, 2020 • edited Loading

anikethsaha left a comment

Choose a reason for hiding this comment

trusktr left a comment

Choose a reason for hiding this comment

anikethsaha commented Jun 22, 2020

jhildenbiddle left a comment

Choose a reason for hiding this comment

anikethsaha commented Jun 22, 2020

mohammedsahl commented Jun 22, 2020 • edited Loading

jhildenbiddle commented Jun 22, 2020 • edited Loading

mohammedsahl commented Jun 23, 2020

jhildenbiddle left a comment

Choose a reason for hiding this comment

mohammedsahl commented Jun 19, 2020 •

edited

Loading

vercel bot commented Jun 19, 2020 •

edited

Loading

codesandbox-ci bot commented Jun 19, 2020 •

edited

Loading

mohammedsahl commented Jun 22, 2020 •

edited

Loading

jhildenbiddle commented Jun 22, 2020 •

edited

Loading